fix: don't charge CoinJoin participants for failures the masternode can't attribute to them - #7568

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/coinjoin/coinjoin.h`:
- Around line 345-350: Update the comments above nSessionDenom to accurately
explain that tinyformat uses the atomic’s implicit int conversion, LogPrint()
accepts it by const reference without copying, and WalletCJLogPrint() forwards
to CWallet::WalletLogPrintf with by-value parameters, requiring an explicit
.load().

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:625-628: Exclude in-flight submissions from guaranteed timeout penalties
  `SelectCollateralToCharge()` only recognizes entries already committed to `vecEntries`, but `AddEntry()` releases `cs_coinjoin` while running `IsCollateralValid()` and `IsValidInOuts()` at lines 759-794. A valid `DSVIN` whose processing began before the deadline can therefore still be validating when the scheduler observes the timeout. With three reservations, two committed entries, and the third entry in flight, the third participant is classified as the only missing submitter, selected with certainty, and charged after `SetNull()` makes its final session revalidation fail. Signing has the same gap while `DSSIGNFINALTX` is being decoded or between its per-input `AddScriptSig()` calls. Track in-flight messages for the current session or serialize the timeout cutoff with their complete processing before applying a guaranteed collateral penalty.
- [BLOCKING] src/coinjoin/server.cpp:610-628: Preserve recoverable finalization when the preceding pool check is skipped
  `CheckTimeout()` assumes the immediately preceding `CheckPool()` handled every recoverable accepting-entry timeout, but `CheckPool()` uses a non-blocking `TRY_LOCK`. A message-handling thread can hold `cs_check_pool`, sample `HasTimedOut()` as false immediately before the deadline, and then release the mutex after the scheduler's `CheckPool()` has skipped it but before the scheduler calls `CheckTimeout()`. `CheckTimeout()` then acquires the mutex after the deadline and unconditionally resets the session. If the session has at least `GetMinPoolParticipants()` committed entries but fewer entries than reservations, it should enter `ChargeAndFinalize` and retain the probabilistic policy; this interleaving instead aborts it and applies `GUARANTEED_ON_ABORT`. Re-evaluate the full accepting-entry action after acquiring `cs_check_pool` rather than relying on a preceding check that may have been skipped or sampled an earlier time.
- [BLOCKING] src/coinjoin/server.cpp:634: Fold the stray-brace correction into its introducing commit
  Commit `67c9647ed03` introduces an extra closing brace immediately after `CCoinJoinServer::CheckTimeout()`. The unmatched brace remains in `264ba3fdfa7`, `76e366e1ceb`, and `9e199087637`, and is only removed by the final commit `2bc5dbd248b`. Those four intermediate commits are syntactically unbuildable and unusable as `git bisect` points. Amend `67c9647ed03` to omit the extra brace, remove the corrective deletion from the final commit, and rebase the intervening commits so every permanent-history state builds independently.
- [SUGGESTION] src/coinjoin/server.cpp:503-509: Make offender deduplication explicit in the commit history
  Commit `9e199087637` is titled `refactor: separate CoinJoin offender selection policy`, but it also changes observable behavior: signing offenders are added once per entry instead of once per unsigned input. This removes the extra random-selection weight previously assigned to participants with multiple unsigned inputs. Move the deduplication hunk into the behavioral fix commit or reword `9e199087637` and add a commit body explaining the weighting change so `git log` and `git blame` expose the functional change.

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:681-684: Do not guarantee a fee after the coordinator aborts signing
  The guaranteed timeout policy applies even after the server has instructed connected clients to stop signing. `RelayFinalTransaction()` calls `RelayStatus(STATUS_REJECTED)` when any entry is disconnected, and `ProcessDSSIGNFINALTX()` does the same after any `AddScriptSig()` failure. Connected clients process that rejection by entering `POOL_STATE_ERROR` and releasing their session resources, but the server remains in `POOL_STATE_SIGNING` unless every entry is disconnected. A malicious participant can exploit this by submitting its valid signature and then resubmitting it: the duplicate fails, every honest peer is told to abort, and the malicious participant is excluded from the unsigned-offender set. At timeout, this block then guarantees that one honest participant is charged. A failed `DSFINALTX` relay can similarly charge a connected entry that never received the transaction. Coordinator-originated signing aborts must reset without the guaranteed fee, or eligibility must be limited to participants that received the final transaction and were not subsequently instructed to abort.
- [BLOCKING] src/coinjoin/server.cpp:669-684: Commit complete signatures when rechecking a timeout
  `CheckTimeout()` re-evaluates recoverable accepting-entry sessions but does not re-evaluate `IsSignaturesComplete()` for signing sessions. A scheduler `CheckPool()` can acquire `cs_check_pool`, observe the signatures as incomplete, and release `cs_coinjoin`. The final on-time `DSSIGNFINALTX` can then add its signature, skip its own `CheckPool()` because the scheduler still owns `cs_check_pool`, and clear its in-flight guard. When the scheduler subsequently enters `CheckTimeout()` after the deadline, there are no unsigned offenders, but this block still calls `SetNull()` and discards the fully signed transaction. Re-evaluate the signing action under `cs_coinjoin`, then call `CommitFinalTransaction()` after releasing that lock, just as the accepting-entry action is re-evaluated and finalized.

In `<commit:aaa6d04>`:
- [SUGGESTION] <commit:aaa6d04>:1: Fold the lint-only follow-up into its introducing commits
  Commit `aaa6d0464a4` only adds `/* Continued */` markers to four `LogPrint` calls introduced earlier in this stack: one in `96cf3768cab` and three in `7443d022e09`. The linter rejects those unmarked calls, so retaining the separate correction leaves the introducing revisions as lint-failing bisect points and adds review-fix noise to permanent history. Fold each marker into the commit that introduced its call and drop `aaa6d0464a4`.

In `<commit:bb056c6>`:
- [SUGGESTION] <commit:bb056c6>:1: Preserve the rationale for the non-obvious CoinJoin fixes
  Commits `bb056c6d397`, `7da61132813`, and `3d5cf6b6e0f` have empty bodies despite changing subtle concurrency and fee-policy invariants. In particular, they serialize timeout reset with finalization, bind entry validation to a session denomination snapshot, and combine guaranteed abort charging with in-flight-message deferral and recoverable-timeout re-evaluation. Add concise bodies explaining the race or policy invariant each commit preserves so ordinary `git log`, blame, and bisect retain the reasoning currently available only from source comments and PR discussion.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/test/coinjoin_inouts_tests.cpp`:
- Around line 750-781: In server_timeout_rechecks_recoverable_session,
explicitly narrow the size_t loop variable when passing it to
MakeCollateral(uint32_t), and explicitly cast the computed address value passed
to MakePeer(NodeId, uint32_t). Preserve the existing loop behavior while
eliminating implicit conversion warnings.
- Around line 188-244: Add EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin) annotations to
ResetForTest, SetTimedOutForTest, AddCollateralForTest, AddEntryForTest,
SelectForTest, EnterSigningState, SetFinalTransactionForTest,
SeedTimedOutSession, and MarkMessageInFlightForTest, matching the existing
annotations on SeedParticipant and RelayAbortForTest. Keep each helper’s
internal cs_coinjoin locking unchanged.
- Around line 321-424: Split server_abort_fee_selects_unique_offenders into
separate BOOST_AUTO_TEST_CASE blocks covering each policy scenario, including
timeout handling, submission coverage, non-signer selection and weighting, the
probabilistic gate, and ChargeRandomFees. Move shared server setup into a small
helper to avoid repetition, and retain each scenario’s existing assertions and
setup behavior.
- Around line 609-624: Replace the floating-point expected amount in the delta
assertion after ApplyDelta with the exact integer expression COIN / 10, while
preserving the existing CAmount comparison and test behavior.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/coinjoin/server.cpp`:
- Around line 348-404: Update CCoinJoinServer::CheckPool’s locked snapshot so
the timeout-driven ChargeAndFinalize action is selected only when
m_inflight_session does not equal the current nSessionID. Preserve normal
finalization when all entries are present, but defer charging/finalizing while
the current session has an in-flight submission, matching the guard used by
CheckTimeout().
- Around line 578-587: Update the probabilistic collateral check using
nSessionCollaterals and vecOffendersCollaterals so it cannot evaluate
nSessionCollaterals - 1 when the size is zero. Add an explicit nonzero guard or
use a signed comparison, while preserving the intended “mostly offending”
behavior when entries exist without collaterals.

---

Nitpick comments:
In `@src/coinjoin/server.cpp`:
- Around line 536-612: Add a unit test in the existing coinjoin input/output
test helpers covering a timed-out session that receives
RelayStatus(STATUS_REJECTED), then verify CheckTimeout() takes the
guaranteed-abort path without consuming or selecting collateral. Reuse the
established session-seeding and relay-rejection setup, and assert the collateral
state remains unchanged after the timeout check.

In `@src/test/coinjoin_inouts_tests.cpp`:
- Around line 180-199: The test-only
TestableCoinJoinServer::consumed_collaterals vector is unsynchronized and could
race if ConsumeCollateral runs on another thread. Protect accesses in
ConsumeCollateral, ResetForTest, and all relevant test assertions with
cs_coinjoin or a dedicated mutex, preserving the existing lifecycle-test
behavior.
- Around line 355-364: Clarify the dual semantics of unsigned_inputs in
MakeEntry: zero creates one signed input, while a positive value creates that
many unsigned inputs. Add a concise comment or rename the parameter to
explicitly communicate this behavior, keeping the existing input-count and
fHasSig logic unchanged.
- Around line 366-486: Extract the repeated CActiveMasternodeManager and
TestableCoinJoinServer setup from the affected test cases into a shared fixture
helper that returns a configured TestableCoinJoinServer. Update each listed
test, including server_abort_fee_selects_unique_offenders, to use the helper
while preserving the existing node dependencies and test behavior; keep
constructor arguments centralized in that helper.
- Around line 709-716: Update the delta assertion in the coinjoin test to use
the shared production prioritisation constant applied by CommitFinalTransaction
instead of hardcoding static_cast<CAmount>(0.1 * COIN). Keep the existing delta
calculation and validation unchanged.

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:382-385: Defer timeout finalization while an entry is in flight
  `ProcessDSVIN()` marks an on-time entry in flight before deserialization and validation, but this timeout-driven `ChargeAndFinalize` branch does not inspect that marker. If the existing committed entries already meet the minimum, the scheduler can select the still-validating participant as a missing submitter, potentially charge its collateral, and move the session to signing. The pending `AddEntry()` then fails its session-state revalidation, excluding an entry that arrived before the deadline. Require `m_inflight_session != nSessionID` for this timeout branch, matching `CheckTimeout()`; normal finalization when every reservation already has an entry should remain unchanged.
- [BLOCKING] src/coinjoin/server.cpp:703-707: Keep new-session admission closed until the penalty is consumed
  `SetNull()` clears the timed-out session and makes the server idle before `ConsumeCollateral()` acquires `cs_main`. During that interval, the message-handling thread can test-accept the selected collateral in `IsAcceptableDSA()` and record it in a replacement session through `CreateNewSession()`. The timeout thread can then consume and relay that collateral, leaving the replacement session with a reservation whose inputs are spent; its participant cannot submit a valid entry, and the new mix can stall or abort. Keep admission closed until consumption finishes, or maintain a pending-penalty prevout set that both new- and existing-session admission reject until the penalty attempt completes.

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:706-710: Keep new-session admission closed until the penalty is consumed
  (existing thread: https://github.com/dashpay/dash/pull/7568#discussion_r3770096427)
  `SetNull()` clears the timed-out session and exposes `POOL_STATE_IDLE` before `ConsumeCollateral()` acquires `cs_main`. A concurrent `DSACCEPT` can therefore pass `IsAcceptableDSA()` while the selected collateral is still unspent and commit it to a replacement session through `CreateNewSession()`; the timeout thread can then consume that collateral, leaving the replacement session with a reservation whose inputs are already spent. The participant cannot subsequently submit a valid entry, so the replacement mix can stall or abort. Keep admission closed until the consumption attempt completes without introducing a `cs_coinjoin` to `cs_main` lock inversion, or retain the selected collateral's prevouts in a pending-penalty set that both new- and existing-session admission reject until consumption finishes.

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:294-310: Revalidate the session before relaying a signature abort
  The failure path does not verify that the session captured in `session_id` is still the current signing session before charging collateral or calling `RelayStatus(STATUS_REJECTED)`. After an extra or duplicate signature passes the initial state check, `CheckPool()` can observe that all required signatures are complete and commit the session without consulting `m_inflight_session`. If the commit resets the session before this message reaches the failure block, `AddScriptSig()` fails against the cleared entries, the collateral lookup finds nothing, and the unconditional `RelayStatus()` sets `m_relayed_abort` while the server is idle. `CreateNewSession()` does not clear that flag, so the next session inherits it and `CheckTimeout()` suppresses its guaranteed abort charge. Revalidate `nSessionID == *session_id` and `nState == POOL_STATE_SIGNING` under `cs_coinjoin` before mutating abort state.

These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.